-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more web storage options #3
base: trunk
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some simple testing with the web_storage: 'cookies'
, seems to work as expected.
I have reviewed 15 out of 18 files. I left some comments below. I have also edited the PR description to fix a code mistake.
In the following line:
sourcebuster-js/src/js/migrations.js
Line 57 in bd51443
// Switch delimiter and renew cookies |
We have a code comment that mentions "cookies", I think it may not be accurate anymore since the code is about web_storage
, so we may need to change the code comment? 🤔
I'll continue review the three remaining helpers
files and do some testings later, if nobody else beat me to it.
#### web_storage | ||
``` javascript | ||
web_storage: 'cookies' | ||
``` | ||
See [PR 3](https://github.com/woocommerce/sourcebuster-js/pull/3) for more details. | ||
|
||
Basically, configure where the Sourcebuster data will be stored and manipulated: | ||
- `cookies` (default) — in multiple cookies | ||
- `singleCookie` — in a single, JSON-encoded cookie | ||
- `localStorage` — in browser `local storage` | ||
- `sessionStorage` — in browser `session storage` | ||
|
||
Any other value (or no value) will default to `cookies`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paragraph spacing, to be consistent with other spacing in the document:
#### web_storage | |
``` javascript | |
web_storage: 'cookies' | |
``` | |
See [PR 3](https://github.com/woocommerce/sourcebuster-js/pull/3) for more details. | |
Basically, configure where the Sourcebuster data will be stored and manipulated: | |
- `cookies` (default) — in multiple cookies | |
- `singleCookie` — in a single, JSON-encoded cookie | |
- `localStorage` — in browser `local storage` | |
- `sessionStorage` — in browser `session storage` | |
Any other value (or no value) will default to `cookies`. | |
#### web_storage | |
```javascript | |
web_storage: 'cookies' | |
```javascript | |
See https://github.com/woocommerce/sourcebuster-js/pull/3 for more details. | |
Basically, configure where the Sourcebuster data will be stored and manipulated: | |
- `cookies` (default) — in multiple cookies | |
- `singleCookie` — in a single, JSON-encoded cookie | |
- `localStorage` — in browser `local storage` | |
- `sessionStorage` — in browser `session storage` | |
Any other value (or no value) will default to `cookies`. |
@@ -12,7 +12,8 @@ var data = { | |||
first_extra: 'sbjs_first_add', | |||
session: 'sbjs_session', | |||
udata: 'sbjs_udata', | |||
promocode: 'sbjs_promo' | |||
promocode: 'sbjs_promo', | |||
single: 'sbjs_current', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have an explanation of the meaning of this single
here? In a documentation, or maybe in this PR description? 🤔
promo: 'code' | ||
promo: 'code', | ||
|
||
single_expire: 'sxp' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have an explanation of the meaning of this single_expire
here? In a documentation, or maybe in the PR description? 🤔
validateType: function( storage_type ) { | ||
// Default to valid_values[0] if storage_type is not in valid_values | ||
var valid_values = ['cookies', 'singleCookie', 'localStorage', 'sessionStorage']; | ||
return valid_values.indexOf( storage_type ) > -1 ? storage_type : valid_values[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use array includes
here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes. Also, I think it is better to be explicit and use 'cookies'
instead of valid_values[0]
.
return valid_values.indexOf( storage_type ) > -1 ? storage_type : valid_values[0]; | |
return valid_values.includes( storage_type ) ? storage_type : 'cookies'; |
case 'cookies': | ||
default: | ||
storage_module = cookies; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying break
for default statement. Some mention it is a good practice. See Break on default case in switch.
case 'cookies': | |
default: | |
storage_module = cookies; | |
case 'cookies': | |
default: | |
storage_module = cookies; | |
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed codes and tested some scenarios. cookies
, singleCookie
and localStorage
seem to work as expected. 👍
📝 💡 When I was testing sessionStorage
, I forgot that it was tied to the browser tab session, and the session_length
value is actually meaningless. I think we should mention it somewhere, or even better, make it work like localStorage
, so that it will expire when it exceeds session_length
or when the browser tab is close.
🚧 In my testing, I have session_length: 1
. When I set web_storage: 'singleCookie'
, I can't seem to get session or "visits" to go greater than 1; it is always 1, and the "Pages seen" is always incrementing. I think this is a bug?
I am leaving 12 comments below. Important ones are marked with ❓ . I think there are a few code mistakes that need to be fixed.
Requesting changes due to the above 🚧 and ❓ .
set: function(name, value, minutes, domain, excl_subdomains) { | ||
// Don't break the build | ||
domain = ''; | ||
excl_subdomains = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since domain
and excl_subdomains
are the last two parameters, we can just remove them and things should still work.
set: function(name, value, minutes, domain, excl_subdomains) { | |
// Don't break the build | |
domain = ''; | |
excl_subdomains = false; | |
set: function(name, value, minutes) { |
destroy: function(name, domain, excl_subdomains) { | ||
// Don't break the build | ||
domain = ''; | ||
excl_subdomains = false; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since domain
and excl_subdomains
are the last two parameters, we can just remove them and things should still work.
destroy: function(name, domain, excl_subdomains) { | |
// Don't break the build | |
domain = ''; | |
excl_subdomains = false; | |
destroy: function(name) { |
// as well as the time when it's supposed to expire | ||
var item = { | ||
value: this.encodeData( value ), | ||
expiry: (new Date()).getTime() + ( Math.max( minutes, local_storage_session_length ) * 60 * 1000 ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Is the Math.max
expected? 🤔 I'm thinking it might give unexpected / undesired behavior.
Consider this scenario:
- We set the session length by calling
setSessionLength( 15 )
, i.e. 15 minutes "default" session length. - We call
set('abc', '123', 5)
, i.e. we intentionally want this item to have 5 minutes expiry. - Because of the
Math.max
, 15 minutes expiry will be set, instead of the 5 minutes expiry that we intentionally want.
I think it may be more sensible to make session length as default parameter, when the minutes
parameter is not supplied by the caller.
I tried to make session length as default parameter like this:
set: function(name, value, minutes = local_storage_session_length ) {
But there is a build error that says:
src/js/helpers/local_storage.js: line 8, col 38, 'default parameters' is only available in ES6 (use 'esversion: 6').
Alternatively, we can try this:
expiry: (new Date()).getTime() + ( Math.max( minutes, local_storage_session_length ) * 60 * 1000 ), | |
expiry: (new Date()).getTime() + ( ( minutes || local_storage_session_length ) * 60 * 1000 ), |
If minutes
is falsy (including 0
), it will use local_storage_session_length
.
This means that with the scenario I mentioned above, when we call set('abc', '123', 5)
, the expiry will be 5 minutes instead of 15 minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If minutes is falsy (including 0), it will use local_storage_session_length.
We could also do an explicit minutes === undefined
check, as sometimes 0
has a convention to denote no limit.
// `item` is an object which contains the original value | ||
// as well as the time when it's supposed to expire | ||
var item = { | ||
value: this.encodeData( value ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ Because we are using a JavaScript object and we are setting value
into item.value
, I'm thinking we don't really need this.encodeData
(and the corresponding this.decodeData
when reading the value). I think we can just do this:
value: this.encodeData( value ), | |
value: value, |
(Note: We can't use object short notation because "'object short notation' is available in ES6".)
In cookies.js file, encodeData
and decodeData
is needed to prevent character collision in the larger document.cookie
string.
domain = ''; | ||
excl_subdomains = false; | ||
|
||
localStorage.removeItem(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ This should be sessionStorage
?
localStorage.removeItem(name); | |
sessionStorage.removeItem(name); |
destroy: function(name, domain, excl_subdomains) { | ||
// Don't break the build | ||
domain = ''; | ||
excl_subdomains = false; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the unneeded parameters:
destroy: function(name, domain, excl_subdomains) { | |
// Don't break the build | |
domain = ''; | |
excl_subdomains = false; | |
destroy: function(name) { |
if( name === data.containers.session ) { | ||
var session = single_cookie[default_cookies.unsbjs(name)]; | ||
if( session ) { | ||
var sts = session.match(/sts=(\d+)/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ I think this should be sxp
, as per data.aliases.single_expire
?
var sts = session.match(/sts=(\d+)/); | |
var sxp = session.match(/sxp=(\d+)/); |
|
||
// Set an expiration for the values | ||
if( name === data.containers.session ) { | ||
value += data.delimiter + data.aliases.single_expire + '=' + (Math.floor(Date.now() / 1000) + minutes * 60); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In local storage and in cookies, we are using time in milliseconds, but for single_cookie here, we are using minutes? I'm thinking it may be better to make them consistent? 🤔
} | ||
} | ||
} | ||
return single_cookie[default_cookies.unsbjs(name)] || default_cookies.get(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we call default_cookies.get(name)
? I'm just thinking / concerned that this might cause issues because we are getting data from two different data structures. 🤔
default_cookies.set(data.containers.single, JSON.stringify(single_cookie), lifetime, domain, isolate); | ||
}, | ||
|
||
deleteOld: function( domain, isolate ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have an explanation or documentation on this deleteOld
function? What's the purpose etc? I don't see it being used in the code, and it is not being mentioned in the PR description.
// if the item doesn't exist, return null | ||
if (!item_raw) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the code would do "if the item is falsy, return null", and localStorage.getItem
returns null
for non-existing items.
Is that the case that we consider falsy values (", 0, false, ...
) "non-existing"?
If so, I'd rather rephrase the code comment to explain from the "why?" perspective, not the "what?" perspective. ("What?" is answered in the code.)
// if the item doesn't exist, return null | |
if (!item_raw) { | |
// We consider items set to a falsy value as nonexistent and return `null` consistently. | |
if (!item_raw) { |
But, I don't know if that's the actual intention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if we want to just forward the behavior, and return null for non existing, I'd rather change the code:
// if the item doesn't exist, return null | |
if (!item_raw) { | |
// if the item doesn't exist, return null | |
if (!item_raw === null) { |
Currently, we do set the item to be stringified JSON object, so it will always be nonfalsy if existing, but I'd rather make the code resilient for future changes. Plus, somebody may use this method to ask for an object set by a different piece of software.
// as well as the time when it's supposed to expire | ||
var item = { | ||
value: this.encodeData( value ), | ||
expiry: (new Date()).getTime() + ( Math.max( minutes, local_storage_session_length ) * 60 * 1000 ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If minutes is falsy (including 0), it will use local_storage_session_length.
We could also do an explicit minutes === undefined
check, as sometimes 0
has a convention to denote no limit.
// if the item doesn't exist, return null | ||
if (!item_raw) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if we want to just forward the behavior, and return null for non existing, I'd rather change the code:
// if the item doesn't exist, return null | |
if (!item_raw) { | |
// if the item doesn't exist, return null | |
if (!item_raw === null) { |
Currently, we do set the item to be stringified JSON object, so it will always be nonfalsy if existing, but I'd rather make the code resilient for future changes. Plus, somebody may use this method to ask for an object set by a different piece of software.
// Select web storage method | ||
storage_init.set(p.web_storage, p.session_length); | ||
web_storage = storage_init.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓
I don't get the rationale behind this architecture, and it looks bug-prone and confusing to me.
- In the PR description, you mentioned, that changing webstorage in runtime is currently not supported, and may result in corrupted data.
And there is nothing in this code that would prevent such corruption, or discourage it anyhow.
sbjs.init()
may be called multiple times (and in WC Core we do so), that technically can happen with different prefs every time. - We set it to immediately get it at the line bellow and get it again inside the
migrations.go
another line bellow.
Do we even need that storage_init
module? Can sbjs.init
just have web_storage
variable and pass it to migrations.go
.
I could imagine it would be useful, if it would not be ..._init
but a storage
singleton, that would prevent from changing it in runtime.
cookies = require('./cookies'); | ||
|
||
module.exports = { | ||
validateType: function( storage_type ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not used anywhere outside, so I'd rather make it private.
This PR enables the user to specify which type of storage to use via the
web_storage
configuration parameter:web_storage: 'cookies'
(default) - multiple cookies as nowweb_storage: 'singleCookie'
- one JSON-encodedsbjs_current
cookie that contains all data stored in the cookiessbjs_current
whitelisted.web_storage: 'sessionStorage'
- keep all data in the browser's session storageweb_storage: 'localStorage'
- keep all the data in the browser's local storageNotes
localStorage
uses the maximum of the session_length (minutes) and lifetime (months converted to minutes) to create an expiry date for the values. Technically they'll live "forever" in the browser's local storage, but whenever Sourcebuster tries to access them, if theexpiry
timestamp has passed, the values will be discarded. This workaround was added to prevent 0-lifetime configuration (all cookies expire with the session) from creating 0-lifetime local storage, which effectively is erased and set every time a page is loaded.Testing
/public/index.html
in a browser.index.html
file, find the Sourcebuster JavaScript configuration. Initially, it should haveweb_storage
set ascookies
.session_length
parameter with value of1
minute if you would like to test transitions from session to session (default is30
minutes).cookies
value forweb_storage
.?utm_source=[VALUE]…
) all capture source values correctly.1
and user sessions increase.sbjs_
cookies) and change theweb_storage
config value tosingleCookie
(camel cased).localStorage
andsessionStorage
options.web_storage
value.